fix(harvester): improve View Changes modal UX on 403 #810
Conversation
TahaKhan998
force-pushed
the
fix/issue-788-view-changes-403-modal
branch
from
d293824 to
19b251d
Compare
TahaKhan998
changed the title
TahaKhan998
force-pushed
the
fix/issue-788-view-changes-403-modal
branch
3 times, most recently
from
19d778a to
5510684
Compare
| onModalTriggerClick = (e, { dataActionKey }) => { | ||
| const { resource } = this.props; | ||
|
|
||
| if (dataActionKey === "view_changes") { |
There was a problem hiding this comment.
AuditLogActions should already have this buttons and the modal trigger right? I am no sure I understand why this override is needed?
There was a problem hiding this comment.
You are right, the base class already has the buttons and opens the modal. The only reason for the override is that the base always puts the default ViewRecentChanges inside the modal and we need our own version that shows a clear message on 403. Everything else now just calls the base handler
| }; | ||
|
|
||
| const renderViewRecentChanges = ViewRecentChanges.prototype.render; | ||
| ViewRecentChanges.prototype.render = function () { |
There was a problem hiding this comment.
I think this can be written in a better and readable way by extending class and overriding the render method instead of injecting a new overriden render?
There was a problem hiding this comment.
You are right, that was a bad way to do it so i changed it to a normal subclass (HarvesterViewRecentChanges extends ViewRecentChanges) that overrides render and fetchPreviousRevision.
TahaKhan998
force-pushed
the
fix/issue-788-view-changes-403-modal
branch
from
5510684 to
6ad2279
Compare
Refactor to subclasses instead of prototype patching per review.
TahaKhan998
force-pushed
the
fix/issue-788-view-changes-403-modal
branch
from
6ad2279 to
bd1b177
Compare
2 participants
Closes #788